-
Notifications
You must be signed in to change notification settings - Fork 725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Finalize grid mode #699
Finalize grid mode #699
Conversation
src/components/MenuBar/Menu/Menu.tsx
Outdated
<CollaborationViewIcon style={{ fill: '#707578', width: '0.9em' }} /> | ||
) : ( | ||
<GridViewIcon style={{ fill: '#707578', width: '0.9em' }} /> | ||
)} | ||
</IconContainer> | ||
<Typography variant="body1">{isGridModeActive ? 'Collaboration Mode' : 'Grid Mode'}</Typography> | ||
<Typography variant="body1">{isGridViewActive ? 'Collaboration View' : 'Grid View'}</Typography> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<Typography variant="body1">{isGridViewActive ? 'Collaboration View' : 'Grid View'}</Typography> | |
<Typography variant="body1">{isGridViewActive ? 'Presentation View' : 'Grid View'}</Typography> |
src/components/GridView/GridView.tsx
Outdated
|
||
const CONTAINER_GUTTER = '50px'; | ||
|
||
const useStyles = makeStyles({ | ||
container: { | ||
background: '#121C2D', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in a few spots, so it would probably be good to add to the theme.
@@ -116,7 +116,7 @@ const useStyles = makeStyles((theme: Theme) => | |||
bottom: 0, | |||
left: 0, | |||
}, | |||
typeography: { | |||
typography: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙂
margin: `${GRID_MODE_MARGIN} - borderWidth`, | ||
}, | ||
mobileGridMode: { | ||
gridView: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding a border-radius of, say, 6px or 8px? Without the white border in grid view (which I think looks great), the corners feel a touch too sharp. I think we could even have presentation view use the same border-radius of 6 or 8px. Or presentation view could stay at 4px. It'll take some playing with but I think that the grid view corners could be more rounded. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, not sure why I haven't noticed this before, but I think the identity
class could use padding of padding: 0.18em 0.3em 0.18em 0;
. Without this, the microphone icon doesn't feel exactly centered. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that all looks a lot better!
@@ -34,14 +34,38 @@ const useStyles = makeStyles((theme: Theme) => { | |||
}; | |||
}); | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but it should be "presentation view" here.
@@ -12,5 +12,5 @@ export const SELECTED_VIDEO_INPUT_KEY = 'TwilioVideoApp-selectedVideoInput'; | |||
// This is used to store the current background settings in localStorage | |||
export const SELECTED_BACKGROUND_SETTINGS_KEY = 'TwilioVideoApp-selectedBackgroundSettings'; | |||
|
|||
export const GRID_MODE_ASPECT_RATIO = 9 / 16; // 16:9 | |||
export const GRID_MODE_MARGIN = 3; | |||
export const GRID_VIEW_ASPECT_RATIO = 9 / 16; // 16:9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we'll need to think about this one. Not sure this needs to be solved in this PR, I but changing this value here doesn't actually change the aspect ratio. I think the fix might be easy though: the container
class in ParticipantInfo doesn't need to enforce an aspect ratio in grid view with padding-top
. Instead it just needs height: 100%.
I realize this is out of scope for this PR, but it had never occured to me to try to change this constant before 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay cool, we can look at this later!
}, | ||
mobileGridMode: { | ||
gridView: { | ||
background: '#121C2D', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -16,16 +16,10 @@ export function usePagination(participants: Participant[]) { | |||
} | |||
}, [isBeyondLastPage, totalPages]); | |||
|
|||
let paginatedParticipants; | |||
let paginatedParticipants = participants.slice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't make a huge difference, but I'm wondering if this hook should be moved into the GridView directory. Just so people don't think it should be used with MobileGridView pagination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea!
src/components/GridView/GridView.tsx
Outdated
@@ -72,23 +74,28 @@ const useStyles = makeStyles({ | |||
alignItems: 'center', | |||
justifyContent: 'center', | |||
}, | |||
lastPage: { | |||
alignContent: 'flex-start', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about centered here? In some cases, flex-start makes the participants on the last page higher than all the rest, like this:
I think centered participants on the last page might be ideal, but I'm curious what you think? It also has the advantage of getting rid of all the lastPage
stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sounds good. I really didn't have an opinion here, and I didn't know what was best. So centering it is!
src/components/Room/Room.test.tsx
Outdated
rerender({ screenShareParticipant: {} } as any); | ||
expect(mockSetIsGridViewActive).toBeCalledWith(false); | ||
// screenshare ends | ||
rerender({ screenShareParticipant: {} } as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rerender({ screenShareParticipant: {} } as any); | |
rerender({ screenShareParticipant: undefined } as any); |
Should correctly end screen share.
src/components/Room/Room.test.tsx
Outdated
expect(mockSetIsGridViewActive).toBeCalledWith(true); | ||
}); | ||
|
||
it('should not reactivate grid view when screenshare ends if grid view was active before another participant started screensharing', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it('should not reactivate grid view when screenshare ends if grid view was active before another participant started screensharing', () => { | |
it('should not activate grid view when screenshare ends if presentation view was active before another participant started screensharing', () => { |
src/components/Room/Room.test.tsx
Outdated
jest.spyOn(React, 'useRef').mockReturnValue({ | ||
current: false, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest.spyOn(React, 'useRef').mockReturnValue({ | |
current: false, | |
}); |
Same as above.
src/components/Room/Room.test.tsx
Outdated
jest.spyOn(React, 'useRef').mockReturnValue({ | ||
current: true, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jest.spyOn(React, 'useRef').mockReturnValue({ | |
current: true, | |
}); |
I don't think this is needed. Here I think we want to test the real functionality of useRef, and not a mocked return value. But also, this code doesn't do much anyway because the mocked value is overwritten by the first useEffect in useSetPresentaionViewOnScreenShare()
src/components/Room/Room.test.tsx
Outdated
expect(mockSetIsGridViewActive).not.toBeCalled(); | ||
rerender({ screenShareParticipant: {} } as any); | ||
expect(mockSetIsGridViewActive).toBeCalledWith(false); | ||
// mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode | ||
expect(mockSetIsGridViewActive).toBeCalledTimes(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the comments from the test above. But also, I think we need to rerender the hook once more to end the screensharing:
expect(mockSetIsGridViewActive).not.toBeCalled();
// screenshare starts
rerender({ screenShareParticipant: {} } as any);
expect(mockSetIsGridViewActive).toBeCalledWith(false);
// screenshare ends
rerender({ screenShareParticipant: undefined } as any);
// mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode
expect(mockSetIsGridViewActive).toBeCalledTimes(1);
// mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode | ||
expect(mockSetIsGridViewActive).toBeCalledTimes(1); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once more test could be nice to test that presentation mode is not activated if the user switches to grid mode during a screenshare:
it('should not activate presentation view when screenshare ends if presentation view was active before screensharing, but the user switched to grid view during the screenshare', () => {
const screenShareParticipant = {};
const room = { localParticipant: {} } as any;
const { rerender } = renderHook(
({ screenShareParticipant, isGridViewActive }) =>
useSetCollaborationViewOnScreenShare(screenShareParticipant, room, mockSetIsGridViewActive, isGridViewActive),
{ initialProps: { screenShareParticipant: undefined, isGridViewActive: false } }
);
expect(mockSetIsGridViewActive).not.toBeCalled();
// start screenshare
rerender({ screenShareParticipant, isGridViewActive: false } as any);
expect(mockSetIsGridViewActive).toBeCalledWith(false);
// enable grid mode
rerender({ screenShareParticipant, isGridViewActive: true } as any);
// stop screenshare
rerender({ screenShareParticipant: undefined, isGridViewActive: true } as any);
// mockSetIsGridViewActive should only be called once with "false" since we're not reactivating grid mode
expect(mockSetIsGridViewActive).toBeCalledTimes(1);
});
I wouldn't mind a more concise description if you can think of one 😆.
Also, note that I saved the room object and screenshare object to their own variables. This is so hook rerenders happen with the same object references as the previous render. This prevents the useEffects from running unnecessarily. This wasn't needed in the previous tests, but it's needed in this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed a couple of words but couldn't really come up with anything much shorter 😢
background: '#000000', | ||
border: `${theme.participantBorderWidth}px solid transparent`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, not sure how I didn't see this before, but it looks like there's a black border around the participants:
I think it's really just a transparent border that's showing the black background. So I guess we just make the border the same color as the grid background color here?
Also I think background: #000000
can be removed here. Looks like there's already background: 'black'
in the container
class.
cy.visit('/'); | ||
cy.visit('/', { | ||
onBeforeLoad: window => { | ||
window.localStorage.setItem('grid-view-active-key', false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a quick comment here about what this does would be good.
@@ -19,6 +19,10 @@ module.exports = (on, config) => { | |||
args, | |||
}); | |||
const page = (participants[name] = await browser.newPage()); // keep track of this participant for future use | |||
await page.evaluateOnNewDocument(() => { | |||
localStorage.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, comment would be good.
Contributing to Twilio
Pull Request Details
JIRA link(s):
Description
This PR makes some final changes to the grid view behavior:
Burndown
Before review
npm test
Before merge